Skip to content
This repository has been archived by the owner on Apr 7, 2022. It is now read-only.

[WIP] Updating CRUD operations test for automate method #8876

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ganeshhubale
Copy link
Member

@ganeshhubale ganeshhubale commented May 28, 2019

PRT RUN

{{ pytest: cfme/tests/automate/test_method.py::test_method_crud -vv }}

@ganeshhubale ganeshhubale changed the title [WIP] Updating CRUD operations test for automate method [WIPTEST] Updating CRUD operations test for automate method Aug 19, 2019
@dajoRH dajoRH changed the title [WIPTEST] Updating CRUD operations test for automate method [WIP] Updating CRUD operations test for automate method Aug 19, 2019
@ganeshhubale ganeshhubale changed the title [WIP] Updating CRUD operations test for automate method [WIPTEST] Updating CRUD operations test for automate method Aug 19, 2019
@ganeshhubale ganeshhubale force-pushed the automate-method-crud branch 3 times, most recently from fe04e8b to 8f0240e Compare August 23, 2019 11:20
@ganeshhubale ganeshhubale changed the title [WIPTEST] Updating CRUD operations test for automate method [RFR] Updating CRUD operations test for automate method Aug 23, 2019
Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few questions, overall looking really nice! 👍

cfme/automate/explorer/method.py Outdated Show resolved Hide resolved
return self.parent_obj.tree_path + [
(icon_name_map[self.location.lower()], '{} ({})'.format(self.display_name,
self.name))]
if self.location in ['Ansible Tower Job Template', 'Ansible Tower Workflow Template']:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on making this strings class level or module level attributes? That way if they need to be updated, you will only have to update them there.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey John,

  • I am not getting what you actually want here. Do you want me to create global list of locations?
  • Please explain more and give me example.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ganeshhubale sorry for delayed reply, I must've missed this. Yeah, I was suggesting that you make global list lof locations, something like:

LOCATIONS =  ['Ansible Tower Job Template', 'Ansible Tower Workflow Template']

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got it @john-dupuy
I will update with this change.

cfme/tests/automate/test_method.py Outdated Show resolved Hide resolved
@john-dupuy john-dupuy changed the title [RFR] Updating CRUD operations test for automate method [WIPTEST] Updating CRUD operations test for automate method Aug 27, 2019
@dajoRH dajoRH changed the title [WIPTEST] Updating CRUD operations test for automate method [WIP] Updating CRUD operations test for automate method Sep 4, 2019
@ganeshhubale ganeshhubale changed the title [WIP] Updating CRUD operations test for automate method [WIPTEST] Updating CRUD operations test for automate method Sep 13, 2019
@ganeshhubale ganeshhubale force-pushed the automate-method-crud branch 2 times, most recently from 1297b8d to cf1c4a4 Compare September 16, 2019 10:21
@ganeshhubale ganeshhubale force-pushed the automate-method-crud branch 3 times, most recently from b9ac4b1 to 39c021f Compare September 23, 2019 12:52
@ganeshhubale ganeshhubale changed the title [WIPTEST] Updating CRUD operations test for automate method [RFR] Updating CRUD operations test for automate method Jan 13, 2020
})
validate = False

if location == 'Ansible Tower Job Template':
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see any difference in add_page.fill for both the locations , why do we need if checks ?

@ganeshhubale ganeshhubale force-pushed the automate-method-crud branch 2 times, most recently from 0121113 to 36b5905 Compare January 14, 2020 07:07
@@ -484,6 +507,15 @@ def create(
'playbook_input_parameters': playbook_input_parameters
})
validate = False

if location in ['Ansible Tower Workflow Template', 'Ansible Tower Job Template']:
Copy link
Contributor

@sshveta sshveta Jan 14, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if location in self.LOCATIONS:

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think location is fine there, no need for self as location is an argument in create.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup that was a typo :)

'method_display_name': display_name,
'ansible_max_ttl': max_ttl,
})
validate = False
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you setting validate=false here ?
Don't we need to validate after fillng data ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sshveta For method types such as 'Ansible Tower Workflow Template', 'Ansible Tower Job Template', 'playbook'; we don't have such feature with button validate. This is applicable only for inline type of method.

)


class Method(BaseEntity, Copiable):

LOCATIONS = ['Ansible Tower Job Template', 'Ansible Tower Workflow Template']
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thoughts on making it global so that you can import it in test file too ?

Copy link
Contributor

@john-dupuy john-dupuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One question, thanks for this PR @ganeshhubale

save_button = Button('Save')
reset_button = Button('Reset')
cancel_button = Button('Cancel')

def before_fill(self, values):
location = self.context['object'].location.lower()
if 'display_name' in values and location in ['inline', 'playbook']:
values['{}_display_name'.format(location)] = values['display_name']
if 'display_name' in values:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ganeshhubale What about the playbook case? Won't this fail if location=='playbook'?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @john-dupuy
Actually I am not getting why this question. So I tested test_automate_ansible_playbook_method_type_crud < this test case with these changes locally. And it works as expected.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ganeshhubale I have some test that uses this method that has failed in this section of code before. I remember I had to add in the location in ['inline', 'playbook'] line to get it to work. Can you please try running test_alert_run_ansible_playbook?

My point is that you've removed the code that handles the case where location == 'playbook'. I.e. to keep the original code intact, L324 should be

if location in ['inline', 'playbook']:

Does that make sense? Or am I missing something here?

elif 'name' in values and location in ['inline', 'playbook']:
values['{}_name'.format(location)] = values['name']

elif 'name' in values:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here about playbook case.

@john-dupuy john-dupuy changed the title [RFR] Updating CRUD operations test for automate method [WIPTEST] Updating CRUD operations test for automate method Jan 16, 2020
@dajoRH
Copy link
Contributor

dajoRH commented Jan 21, 2020

I detected some fixture changes in commit 554ce24

The local fixture original_class is used in the following files:

  • cfme/tests/automate/test_method.py
    • test_automate_simulate_retry

Please, consider creating a PRT run to make sure your fixture changes do not break existing usage 😃

@dajoRH dajoRH changed the title [WIPTEST] Updating CRUD operations test for automate method [WIP] Updating CRUD operations test for automate method Mar 31, 2020
@dajoRH
Copy link
Contributor

dajoRH commented Mar 31, 2020

Would you mind rebasing this Pull Request against latest master, please? :trollface:
CFME QE Bot

@dajoRH dajoRH added WIP and removed WIP-testing labels Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants